-
Notifications
You must be signed in to change notification settings - Fork 30
fix JitCall codegen for function with lambda as return value #678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #678 +/- ##
==========================================
+ Coverage 79.68% 79.74% +0.06%
==========================================
Files 9 9
Lines 3942 3955 +13
==========================================
+ Hits 3141 3154 +13
Misses 801 801
🚀 New features to boost your workflow:
|
clang-tidy review says "All clean, LGTM! 👍" |
lib/CppInterOp/CppInterOp.cpp
Outdated
bool IsLambdaClass(TCppType_t type) { | ||
QualType QT = QualType::getFromOpaquePtr(type); | ||
if (auto* CXXRD = QT->getAsCXXRecordDecl()) { | ||
return CXXRD->isLambda(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI isLambdaCallOperator
in clang.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isLambdaCallOperator
takes a CXXMethodDecl
, but we are using QualType
to check here.
Also, we are not checking if the function is the lambda call operator; instead, we are checking if the given class is a lambda class.
3cc7e4f
to
aada7a9
Compare
clang-tidy review says "All clean, LGTM! 👍" |
aada7a9
to
1a8c719
Compare
clang-tidy review says "All clean, LGTM! 👍" |
d087d12
to
816474a
Compare
clang-tidy review says "All clean, LGTM! 👍" |
Hi @anutosh491, @mcbarton, |
Weird ..... it says the exception handling test fails. I can recreate the exact same example in the lite deployment CppInterOp/unittests/CppInterOp/InterpreterTest.cpp Lines 155 to 179 in 515c1c8
![]() |
@anutosh491 could this have anything to do with the fact that exception handling in CppInterOps deployment doesn't currently work (I'm assuming your example comes from xeus-cpps deployment)? That requires this PR to go in #686 (patch taken from Emscripten forge). |
No it shouldn't matter (unless anything in createinterpreter or process changed) |
I think this can be |
816474a
to
1f8e469
Compare
clang-tidy review says "All clean, LGTM! 👍" |
@vgvassilev @Vipul-Cariappa if you look at this job https://github.com/compiler-research/CppInterOp/actions/runs/16648661110/job/47122629847?pr=678 it passes the test other Emscripten jobs fail for. The only difference between this job and the other llvm 20 Emscripten jobs is that this patch has been applied on main https://github.com/compiler-research/CppInterOp/blob/main/patches/llvm/emscripten-clang20-3-enable_exception_handling.patch . I hadn't updated all the llvm 20 caches yet, since it quite disruptive and I planned to do it this evening when things are quiet. Once I have updated the llvm 20 Emscripten cache on main, then all the llvm 20 jobs should then pass. To get all the jobs to pass you would need to backport this patch to llvm 19. |
clang-tidy review says "All clean, LGTM! 👍" |
@Vipul-Cariappa @vgvassilev The llvm 19 Emscripten jobs are now passing the test they were previously failing for, now the exception handling patch has been backported. The ci should hopefully pass now. |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
334b41d
to
7d3e9dd
Compare
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
Fixes # (issue)
Helps fix a test in cppyy
Type of change
Please tick all options which are relevant.
Testing
Checklist